Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Polling adapter and example #932

Merged
merged 3 commits into from
Jul 7, 2022
Merged

Conversation

kristjanvalur
Copy link
Contributor

An adaptor to use redisAsyncContext using simple polling. Useful in frameworks where an IO loop isn't present.

@michael-grunder
Copy link
Collaborator

Hi, @kristjanvalur thanks for the PR. I'll take a look at the logic this week.

@kristjanvalur
Copy link
Contributor Author

Right. This is extracted from some work I did to make this jive from within UnrealEngine.

@michael-grunder
Copy link
Collaborator

I rebased your PR into a single commit and made a few (mostly formatting) changes/suggestions in a branch on my fork. Mostly it's just to maintain hiredis' coding style.

The only actually changed logic:

  1. Move timeval -> double (and vice versa) conversion into helpers.
  2. Create REDIS_POLL_XXX defines to signal which events were processed in redisPollTick

If you're OK with the changes feel free to apply the patch as whitespace shouldn't make me the author.

One question I did have, was whether we may wish to handle the case where select returns -1 and sets errno?

@kristjanvalur
Copy link
Contributor Author

Great, I'll have a look, and think about the question

@kristjanvalur
Copy link
Contributor Author

Ok, yes, IMHO we should check for -1 out of select() . Since the socketcompat.h aldready has a helper for poll(), I can change the code to use poll and we get the platform compatible error handling for free.

@kristjanvalur
Copy link
Contributor Author

poll() also has a much nicer API than select. I'll never touch select() again :)

@kristjanvalur
Copy link
Contributor Author

kristjanvalur commented Nov 30, 2021

I've cherry picked the initial async tests I had added in pr #948
At the time, there were no async tests in test.c. Now I see that there is one test, using libevent. The advantage of using polling adapter is that there is no external dependency and no mysterious external event loop to deal with. It also requires no special #ifdef for that reason.

#948 already has this test, and tests for a number of other async issues that I fixed and are rolled up in that PR.
Feel free to object, and I can remove the unit test again, or point out any other issues with it.

@kristjanvalur
Copy link
Contributor Author

Going to clean up the tests a little and add basic connection tests...

@kristjanvalur
Copy link
Contributor Author

So, any news here? IMHO this has been ready for a while now.

@kristjanvalur
Copy link
Contributor Author

This PR would be highly appreciated. For one thing, writing unit-tests using the polling method is much simpler, since it doesn't require any additional scaffolding or external libraries.

If there is anything I need to fix up (style, formatting, collapsing changes, etc) I'm happy to do so.

@michael-grunder
Copy link
Collaborator

If you wouldn't mind rebasing off of master and squashing the polling adapter commits and async test commits where possible, that would be great. Since I have a commit in the middle a full squash is probably not practical.

I took a crack at it on my fork, and thankfully it wasn't too painful.

@kristjanvalur
Copy link
Contributor Author

Its now been cleaned and collapsed into a couple of distinct commits. I'm unsure about whatever formatting concerns you might have, what style rules should be followed.

@michael-grunder michael-grunder merged commit eaa2a7e into redis:master Jul 7, 2022
@michael-grunder
Copy link
Collaborator

Merged, thanks!

@kristjanvalur kristjanvalur deleted the pr3 branch July 8, 2022 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants